Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLI Tools, CLI for Steps 1-7 #88

Merged
merged 73 commits into from
Feb 17, 2024

Conversation

cpaniaguam
Copy link
Member

To facilitate reviewing #78, we are splitting it into bite-size chunks. This is the first one.

Changes:

  1. Update: Dependencies and CI Runner for Step 1
    Add typer and others. The CI runner for Step 1 has also been updated to reflect these changes.

  2. Feature: CLI Tools for Step 1
    CLI tools for the package and the particular cli for Step 1. It includes argument parsing and validation, and improved feedback output.

@cpaniaguam cpaniaguam linked an issue Jan 31, 2024 that may be closed by this pull request
@cpaniaguam cpaniaguam self-assigned this Jan 31, 2024
Copy link
Member

@hollandjg hollandjg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Carlos, thanks for the PR! I've taken a look and I have a few comments and questions – perhaps we can talk this through tomorrow?

.github/workflows/test-B01_SL_load_single_file.yml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
src/icesat2_tracks/analysis_db/B01_SL_load_single_file.py Outdated Show resolved Hide resolved
src/icesat2_tracks/analysis_db/B01_SL_load_single_file.py Outdated Show resolved Hide resolved
src/icesat2_tracks/clitools.py Outdated Show resolved Hide resolved
src/icesat2_tracks/clitools.py Outdated Show resolved Hide resolved
src/icesat2_tracks/clitools.py Outdated Show resolved Hide resolved
src/icesat2_tracks/clitools.py Show resolved Hide resolved
src/icesat2_tracks/clitools.py Outdated Show resolved Hide resolved
Also, add some docstrings and doctests
@cpaniaguam cpaniaguam requested a review from hollandjg February 5, 2024 21:34
kmilo9999
kmilo9999 previously approved these changes Feb 6, 2024
Copy link
Collaborator

@kmilo9999 kmilo9999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me.
I left a comment below about the warnings

Not meant to be used with cli with non-cli scripts
hollandjg
hollandjg previously approved these changes Feb 12, 2024
Copy link
Member

@hollandjg hollandjg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Comment on lines 12 to 22
def suppress_stdout(verbose=False):
if verbose:
yield
else:
with open(os.devnull, "w") as devnull:
old_stdout = sys.stdout
sys.stdout = devnull
try:
yield
finally:
sys.stdout = old_stdout
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a function provided in python 3.4+ which does this – please use that instead!

Check out https://stackoverflow.com/a/37989355/5542581:

import contextlib

with contextlib.redirect_stdout(None):
    print("will not print")

print("this will print")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, thanks for the suggestion @hollandjg! I adapted the current implementation from a context manager that was available elsewhere in the repo.

It seems like contextlib.redirect_stdout is somewhat limited for this use case, however, as it doesn't have the flexibility to do conditional suppression. Can you think of a simple way to refactor the currently implemented suppress_stdout so that it leverages contextlib.redirect_stdout? I gave it some thought but couldn't come up with anything straightfoward.

cpaniaguam and others added 20 commits February 12, 2024 14:58
…ds_priorpy

feat: cli step 4 a02c iowaga thredds prior
…ovpy

feat: cli step 3 -- b03_plot_spectra_ovpy
feat: add cli to step 2 -- B02_make_spectra_gFT.py
@cpaniaguam cpaniaguam dismissed stale reviews from hollandjg and kmilo9999 via f3c28c7 February 17, 2024 06:40
@cpaniaguam cpaniaguam changed the title CLI Tools, CLI for Step 1 CLI Tools, CLI for Steps 1-7 Feb 17, 2024
@cpaniaguam cpaniaguam merged commit bc3d591 into main Feb 17, 2024
1 check passed
@cpaniaguam cpaniaguam deleted the 87-cli-step-1-b01_sl_load_single_filepy branch February 17, 2024 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧹 Code cleanup enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI: step 1 -- B01_SL_load_single_file.py
3 participants